-
-
Notifications
You must be signed in to change notification settings - Fork 629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade Optuna from v2.x.x to v3.0.0 #2360
base: main
Are you sure you want to change the base?
Upgrade Optuna from v2.x.x to v3.0.0 #2360
Conversation
Seems that the lint fails even for the main https://app.circleci.com/pipelines/github/facebookresearch/hydra/14763/workflows/e495a59a-f45c-4531-a51a-898dad566d50/jobs/146566 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @keisuke-umezawa!
Do you expect that this change to the optuna sweeper plugin will be backwards compatible for Hydra users?
Could you please bump the version in hydra_optuna_sweeper/hydra_plugins/hydra_optuna_sweeper/__init__.py
to "1.3.0.dev0"
?
plugins/hydra_optuna_sweeper/hydra_plugins/hydra_optuna_sweeper/_impl.py
Outdated
Show resolved
Hide resolved
Thank you for your comment!
If the meaning of the backwards compatibility here is that we can run hydra with both Optuna v2.x and v3.x, we should not merge this PR. This code change can run only with Optuna v3.x. As Optuna will keep the backwards compatibility for a while, e.g. until v4, we do not need to merge this PR at the moment. But, in that case, one note is that someone needs to merge this PR when Optuna will totally break the backwards compatibility. |
My question is about the Optuna Sweeper's user API: will Hydra users need to update their yaml configs?
This is fine :) |
I will push some commits to this PR branch to try and make the CI pass. |
Your understanding is correct. They do not need to change it.
Thank you! You can merge it when it is appropriate. |
It is possibile to install using pip a dev version of hydra sweeper with support for optuna 3 ? |
Could this please be merged? The 2.1 version of Optuna is very much out-of-date. |
I'd also like this to be merged! |
@Jasha10 |
…r/_impl.py Co-authored-by: Jasha10 <[email protected]>
Hi @keisuke-umezawa, Since it's been a while, it would be good to rebase this diff on top of the Currently I'm no longer an official maintainer of Hydra. That being said, I do hope this diff can be merged at some point. |
You can pip-install the version of
Or you can git-clone the repo, check out @keisuke-umezawa's |
ac16bc5
to
900dbff
Compare
@Jasha10 |
No problem @keisuke-umezawa. I believe that @omry and @shagunsodhani are current maintainers of Hydra. I believe the CI failures here are unrelated to this diff. Once #2692 and #2693 are fixed, it should be possible to get the CI green. |
Are there any updates on this? It seems the issues mentioned by @Jasha10 are all merged. I would love to use optuna >= 3 . |
In the meantime you should be able to install a version with the fix with the command in #2360 (comment) |
900dbff
to
0bf1a98
Compare
0bf1a98
to
0bf382b
Compare
I would like to bump this PR :) Can I help with any requested changes? |
Motivation
(Write your motivation for proposed changes here.)
To upgrade Optuna from v2.x.x to v3.0.0
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
(How should this PR be tested? Do you require special setup to run the test or repro the fixed bug?)
By the existing unit tests
Related Issues and PRs
(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)
Fixes #2162